drpcmetadata: Add support for lowercase keys, fast metadata lookup and other helper methods #21
Conversation
|
I understand the PR is in draft, but I thought to leave some comments before go further
Is there any use case in cockroach where we need this? If not, I think we should avoid it. |
drpcmetadata/metadata.go
Outdated
| // NewOutgoingContext attaches new metadata onto an outgoing context and returns | ||
| // the context. Same as NewIncomingContext for now, | ||
| // as we don't have separate keys for incoming and outgoing metadata. | ||
| // Will be fixed as part https://github.com/cockroachdb/cockroach/issues/156444 | ||
| func NewOutgoingContext(ctx context.Context, | ||
| metadata map[string]string) context.Context { | ||
| newMetadata := make(map[string]string) | ||
| for k, v := range metadata { | ||
| newMetadata[strings.ToLower(k)] = v | ||
| } | ||
| return context.WithValue(ctx, metadataKey{}, newMetadata) |
There was a problem hiding this comment.
I think this change should go in a separate PR. And how much effort would be to separate the keys and close the referenced issue since we are at it.
There was a problem hiding this comment.
Separating keys is not in the scope of these changes and it's not clear at this point if I'd take them up, but I'd prefer to add this method here with a note that it is WIP, since it helps keep the semantics in the dependent calling code clear. I have another draft PR with the changes in the calling code which I will share soon.
Agreed! This PR is in draft because some of the changes in here are provisional (including this one). I'm working on the side to establish that no existing use-case would be broken due to this issue and will update the PR accordingly. |
Thanks for the early feedback 👍 |
…d other helper methods This change has the following features: - Support for lowercase-only keys in metadata. This is mandated by grpc due to requirements imposed by HTTP/2, and is not really needed in drpc but is being added for backward compatibility. - Helper method FastFromIncomingContext for fast metadata retrieval, similar to the one in fast_metadata.go (cockroach repo). - NewIncomingContext and NewOutgoingContext to create context with new metadata. They are both the same for now, as we don't have separate keys for incoming and outgoing metadata in drpc. However, adding them would help with clarity in the calling code. The implementation for these can be fixed as part of as part cockroachdb/cockroach#156444
suj-krishnan
force-pushed
the
enhance_drpc_metadata
branch
from
34ece18 to
9ebe8c9
Compare
- split incoming and outgoing - add new helper methods
2 participants
This PR has the following changes towards fixing cockroachdb/cockroach#157774. Wherever applicable, it assumes the same semantics as the grpc metadata package.
FastFromIncomingContextfor fast metadata retrieval, similar to https://github.com/cockroachdb/cockroach/blob/65848d04fa089a5197aac2b4b1e0cf5e7afae060/pkg/util/grpcutil/fast_metadata.go#L26NewIncomingContextandNewOutgoingContextto create context with new metadata. They are both the same for now, as we don't have separate keys for incoming and outgoing metadata in drpc. However, adding them would help with clarity in the calling code. The implementation for these can be fixed as part of drpc: separate the incoming and outgoing keys for the drpcmetadata cockroach#156444